Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tls: Reset secureConnecting on client socket #33209

Closed
wants to merge 1 commit into from
Closed

tls: Reset secureConnecting on client socket #33209

wants to merge 1 commit into from

Conversation

davedoesdev
Copy link
Contributor

secureConnecting is never set to false on client TLS sockets.
So if Http2Session constructor (in lib/internal/http2/core.js) is
called after secureConnect is emitted, then it will wrongly wait
for a secureConnect event.

This fix sets secureConnecting to false when a client TLS socket
has connected.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@davedoesdev
tls: Reset secureConnecting on client socket
3c4aa3d 
secureConnecting is never set to false on client TLS sockets.
So if Http2Session constructor (in lib/internal/http2/core.js) is
called after secureConnect is emitted, then it will wrongly wait
for a secureConnect event.

This fix sets secureConnecting to false when a client TLS socket
has connected.
@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label May 2, 2020
@davedoesdev
Copy link
Contributor Author

Note I get a tonne of C++ linter errors when I run make test (I get them even on master) but the test passes for me.

@davedoesdev
Copy link
Contributor Author

Without the fix, the test hangs.

@BridgeAR BridgeAR requested review from ronag and sam-github May 3, 2020 15:35
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 7, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31220/

addaleax pushed a commit that referenced this pull request May 9, 2020
@davedoesdev @addaleax
tls: reset secureConnecting on client socket
ea465fa 
secureConnecting is never set to false on client TLS sockets.
So if Http2Session constructor (in lib/internal/http2/core.js) is
called after secureConnect is emitted, then it will wrongly wait
for a secureConnect event.

This fix sets secureConnecting to false when a client TLS socket
has connected.

PR-URL: #33209
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@addaleax
Copy link
Member

addaleax commented May 9, 2020

Landed in ea465fa, thanks for the PR! 🎉

@addaleax addaleax closed this May 9, 2020
codebytere pushed a commit that referenced this pull request May 11, 2020
@davedoesdev @codebytere
tls: reset secureConnecting on client socket
eedc131 
secureConnecting is never set to false on client TLS sockets.
So if Http2Session constructor (in lib/internal/http2/core.js) is
called after secureConnect is emitted, then it will wrongly wait
for a secureConnect event.

This fix sets secureConnecting to false when a client TLS socket
has connected.

PR-URL: #33209
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@codebytere codebytere mentioned this pull request May 18, 2020
Merged
@codebytere
Copy link
Member

This applies cleanly to v12.x but breaks a test:

=== release test-http2-connect ===                                            
Path: parallel/test-http2-connect
events.js:292
      throw er; // Unhandled 'error' event
      ^

Error: read ECONNRESET
    at TLSWrap.onStreamRead (internal/stream_base_commons.js:205:27)
Emitted 'error' event on TLSSocket instance at:
    at emitErrorNT (internal/streams/destroy.js:92:8)
    at emitErrorAndCloseNT (internal/streams/destroy.js:60:3)
    at processTicksAndRejections (internal/process/task_queues.js:84:21) {
  errno: 'ECONNRESET',
  code: 'ECONNRESET',
  syscall: 'read'
}
Command: out/Release/node /Users/codebytere/Developer/node/test/parallel/test-http2-connect.js

I can open a manual backport but might need some help tracking down the issue.

@blakeembrey blakeembrey mentioned this pull request Jun 11, 2020
Merged
@pimterry pimterry mentioned this pull request Aug 18, 2020
Closed
@addaleax addaleax mentioned this pull request Aug 20, 2020
Closed
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 25, 2020
@davedoesdev @addaleax
tls: reset secureConnecting on client socket
6ff8562 
secureConnecting is never set to false on client TLS sockets.
So if Http2Session constructor (in lib/internal/http2/core.js) is
called after secureConnect is emitted, then it will wrongly wait
for a secureConnect event.

This fix sets secureConnecting to false when a client TLS socket
has connected.

PR-URL: nodejs#33209
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
@davedoesdev @MylesBorins
tls: reset secureConnecting on client socket
66a434b 
secureConnecting is never set to false on client TLS sockets.
So if Http2Session constructor (in lib/internal/http2/core.js) is
called after secureConnect is emitted, then it will wrongly wait
for a secureConnect event.

This fix sets secureConnecting to false when a client TLS socket
has connected.

Backport-PR-URL: #34859
PR-URL: #33209
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
@davedoesdev @MylesBorins
tls: reset secureConnecting on client socket
12d76b8 
secureConnecting is never set to false on client TLS sockets.
So if Http2Session constructor (in lib/internal/http2/core.js) is
called after secureConnect is emitted, then it will wrongly wait
for a secureConnect event.

This fix sets secureConnecting to false when a client TLS socket
has connected.

Backport-PR-URL: #34859
PR-URL: #33209
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@sam-github sam-github sam-github approved these changes

@ronag ronag Awaiting requested review from ronag

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@davedoesdev @nodejs-github-bot @addaleax @codebytere @sam-github @lpinca